BREAKING(http/unstable): add capability to attach handlers by methods, implement sensible defaults #6305
Conversation
…ment sensible defaults
|
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6305 +/- ##
=======================================
Coverage 96.33% 96.34%
=======================================
Files 547 547
Lines 41667 41681 +14
Branches 6314 6316 +2
=======================================
+ Hits 40140 40157 +17
+ Misses 1484 1482 -2
+ Partials 43 42 -1 ☔ View full report in Codecov by Sentry. |
kerezsiz42
changed the title
kt3k
changed the title
| } | ||
| } | ||
| return defaultHandler(request, info); | ||
|
|
||
| return new Response("Not Found", { status: 404 }); |
There was a problem hiding this comment.
I don't think it's a good idea to remove the capability to customize the 404 page
There was a problem hiding this comment.
I think I mentioned it in the PR but did not emphasize it enough. So putting a wildcard route that matches any URLPattern at the end of the Route[] achieves exactly the same result as the current defaultHandler:
const wildcardRoute: Route = {
pattern: new URLPattern({ pathname: "/*" }),
handler: (request: Request) => {
return new Response(new URL(request.url).pathname, { status: 404 });
},
};For this reason I think that the defaultHandler is unnecessary, and the second parameter of route could be deleted or changed to something else without losing features.
|
Thanks for the suggestion. The addition of errorHandler option seems interesting to me. But we probably don't need the default implementation as
I don't see much benefit of this change. This design seems rather confusing to me as |
Thanks for the code review! You did not criticize what I thought you would. So if I understand you correctly, only the naming of I understand that this module of the std library is not on agenda currently, but we, as library developers should be concentrating on what the users of the standard library want. And a high quality HTTP toolkit is of utmost importance as we see it in case of Go. For new JS/TS projects the default option for new users should be I am new to open source development, so I might not be familiar with how this kind of development really works. I am keen to hearing your advice and opinion! |
Hi everyone!
I would like to propose a new implementation of the
route()function that adds the capability to define handlers on a per-method basis (in a style similar to Fresh) or to use a single handler that handles all methods.I envision the
route()function being used only once when we pass it toDeno.serve(). In the case of a REST API, one would:user.tswith URL patterns such as/api/usersand/api/users/:id.products.tswith URL patterns such as/api/productsand/api/products/:id.main.ts, then callDeno.serve(route([...userRoutes, ...productRoutes, notFoundRoute], errorHandler)).The current
defaultHandler()does not seem very useful, as a wildcard route can provide the same functionality (see tests). I believe an optionalerrorHandler()would be more important.While my PR adds some logic to the
route()function, it conforms to the relevant specifications (RFC2616) and practices regarding the "Method Not Allowed" response, error handling, and the default "Not Found" response, at least to my understanding. Furthermore, all of the new default behavior remains customizable by the user.Accepting this PR in its current form would essentially mean that the
URLPatternwould be the only factor determining whether a Route can handle the request or not.